Can you explain why was this needed before, and now it is not? 🤔 What are the chances of this producing some regressions? 🤔

Isn't there a chance that Data will be missing or the FRC won't trigger updates?

Yes, we need more details on why was this removed.

I looked into the history of this and it goes like this:

1. In 2021 it started as an optimisation change for reducing context changes

// If you touch ManagedObject and update one of it properties to same value
// Object will be marked as `updated` even it hasn't changed.
// By reseting such objects we remove updates that are not updates.
for object in self.writableContext.updatedObjects {
    if object.changedValues().isEmpty {
        self.writableContext.refresh(object, mergeChanges: false)
    }
}

refresh(_:mergeChanges:false) = "Updates the persistent properties of a managed object to use the latest values from the persistent store. If flag is false, the context discards pending changes and the managed object becomes a fault. Upon next access, the context reloads the object’s values from the persistent store or last cached state."

2. In early 2023 it gets changed to
   self.writableContext.refresh(object, mergeChanges: true)
   "If flag is true, the context reloads the object’s property values from the store or the cache. Then the context applies local changes over the newly loaded values. Merging the local values into object always succeeds, and never results in a merge conflict."
   I think this was done because change 1 was making FRC to lose track of changes.
   Reasoning:
   We use DTOs' willSave to force FRC updates by accessing the "parent" DTO and assigning a property and setting back the same value to the DTO. For example, UserDTO is getting saved and if it happens to be CurrentUserDTO.user, then UserDTO.willSave forces the CurrentUserDTO to change as well. Then FRC observing the CurrentUserDTO picks up the change. Discarding changes in these cases breaks FRC observing which I think the change number 2 tackled. The change in number 2 made sure these updates go through.
   From UserDTO:

if let currentUser = currentUser, !currentUser.hasChanges {
    let fakeNewUnread = currentUser.unreadChannelsCount
    currentUser.unreadChannelsCount = fakeNewUnread
}

3. I removed the if check when testing if it fixes these some crashes in CoreData. It didn't.

Since the change number 2 some things have happened like we do not use viewContext anymore (except reading the state in DataStore, DB observers do not use it anymore). All the DB mutations go through the writableContext. Therefore, there should never be a case where writableContext itself would require to refresh the DTO from the persistent store (because it is the only one changing it). That is the main reason I believe we can remove it.
Interesting side of it is that refreshing an object seems to discard cached data in Core Data (cached data in other contexts) which leads to performance degradation.

Didn't we also introduce a state layer context? Will this have an impact on it, have you checked that?

For state layer, with similar test, the change is 1.60 s to 0.77 s (as expected)

In the performance benchmark table, we are not checking the memory usage. Is there any significant increase in memory by caching all the extra data?

I don't expect it, but maybe we clean it up on memory warning notifications, just in case?

Internally it is NSCache which clears itself on low memory warning. I used a wrapping class because of unit-tests. Can't mock NSCache from the Swift side (it is objc generic class and brings restrictions).

There is significant decrease in memory usage used for RawJSON.
Before:

After

Another interesting thing that we could try to improve is that ChatMember and ChatUser are both classes. And I think we should be able to convert them to structs without API changes. Could be interesting to see if there are any performance improvements by doing so 🤔

why do you think structs would improve performance? Probably only the SDK size would increase.

ChatUser.init itself is visible in the time profiler, but it is like 1 ms which is nothing compared to other things happening. At some point it would be nice to only use structs for models (consistent).

Usually structs are lightweight to create and destroy, so it should improve things, but maybe not significantly. @laevandus We not only need to check the init, but also how much time is spend destroying existing objects 🤔

Based on time profiler I don't see this popping up really, a couple of ms for ChatUser.__deallocating_deinit and ChatChannelMember.__deallocating_deinit. We should do that for consistency, but in a separate ticket. No real performance improvement here.

Okay nice 👍

why do you think structs would improve performance? Probably only the SDK size would increase.

Yes, we need more details on why was this removed.

I don't expect it, but maybe we clean it up on memory warning notifications, just in case?